Skip to content

Conversation

pamelafox
Copy link
Collaborator

@pamelafox pamelafox commented Sep 8, 2025

Purpose

Fixes #2710

Our frontend was setting the multimodal-parameters sendImageSources and searchImageEmbedding parameters to true, which caused an error in backends with multimodal disabled.

This corrects the frontend to default to False AND adds an extra guard to backend to ensure they can only be set to true if multimodal is enabled.

Also made sure /ask supports send_text_sources like /chat does, the logic was missing.

Test wise, I adjusted an e2e test to confirm the defaults, and added two backend tests.

Does this introduce a breaking change?

When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.

[ ] Yes
[X] No

Does this require changes to learn.microsoft.com docs?

This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.

[ ] Yes
[X] No

Type of change

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

Code quality checklist

See CONTRIBUTING.md for more details.

  • The current tests all pass (python -m pytest).
  • I added tests that prove my fix is effective or that my feature works
  • I ran python -m pytest --cov to verify 100% coverage of added lines
  • I ran python -m mypy to check for type errors
  • I either used the pre-commit hooks or ran ruff and black manually on my code.

Copilot

This comment was marked as outdated.

text_sources.append(f"{citation}: {nonewlines(' . '.join([cast(str, c.text) for c in doc.captions]))}")
else:
text_sources.append(f"{citation}: {nonewlines(doc.content or '')}")
if include_text_sources:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change here is the very top line-

if download_image_sources:
return DataPoints(text=text_sources, images=image_sources, citations=citations)
return DataPoints(text=text_sources, citations=citations)
return DataPoints(text=text_sources, images=image_sources, citations=citations)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes image_sources [] when not including image sources, which is consistent with text_sources being [] when not including text sources. Snapshots already aligned with this, and it simplified code.

// Set default vector field settings
setSearchTextEmbeddings(true);
setSearchImageEmbeddings(true);
// Initialize from server config so defaults follow deployment settings
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how /chat was already doing it, fixed for consistency

@pamelafox pamelafox requested a review from Copilot September 8, 2025 20:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes multimodal-related parameter defaults to prevent errors in deployments without multimodal features enabled. The frontend was incorrectly defaulting multimodal parameters to true, causing backend errors when multimodal was disabled.

Key changes:

  • Updated frontend defaults for sendImageSources and searchImageEmbeddings from true to false
  • Added backend guards to ensure multimodal parameters can only be enabled when multimodal is actually available
  • Fixed missing send_text_sources support in the /ask endpoint to match /chat functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
app/frontend/src/pages/chat/Chat.tsx Changed default values for image-related parameters from true to false
app/frontend/src/pages/ask/Ask.tsx Changed default values for image-related parameters from true to false and updated initialization logic
app/backend/approaches/chatreadretrieveread.py Added multimodal guards and support for send_text_sources parameter
app/backend/approaches/retrievethenread.py Added multimodal guards and support for send_text_sources parameter
app/backend/approaches/approach.py Updated get_sources_content method to handle include_text_sources parameter
tests/test_app.py Added tests for send_text_sources and multimodal parameter validation
tests/e2e.py Updated e2e test to validate correct default parameter values

if download_image_sources:
return DataPoints(text=text_sources, images=image_sources, citations=citations)
return DataPoints(text=text_sources, citations=citations)
return DataPoints(text=text_sources, images=image_sources, citations=citations)
Copy link
Preview

Copilot AI Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return statement always includes image_sources even when download_image_sources is False. This should conditionally include images only when download_image_sources is True, otherwise pass None or an empty list for the images parameter.

Suggested change
return DataPoints(text=text_sources, images=image_sources, citations=citations)
return DataPoints(
text=text_sources,
images=image_sources if download_image_sources else None,
citations=citations
)

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot But it will be an empty list in that case, since image_sources is only populated when download_image_sources is True?

search_index_filter = self.build_filter(overrides, auth_claims)
send_text_sources = overrides.get("send_text_sources", True)
send_image_sources = overrides.get("send_image_sources", True)
send_image_sources = overrides.get("send_image_sources", self.multimodal_enabled) and self.multimodal_enabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great :)

@pamelafox pamelafox merged commit 74b66bd into Azure-Samples:main Sep 8, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default deployment variables assumes multimodal embeddings use
2 participants